Skip to content

Fix critical bug in truncateStringWithMessage: Prevent negative slice indices - #1209

Open
pavankumar-vh wants to merge 2 commits into
CodebuffAI:mainfrom
pavankumar-vh:fix/string-truncation-bug
Open

Fix critical bug in truncateStringWithMessage: Prevent negative slice indices#1209
pavankumar-vh wants to merge 2 commits into
CodebuffAI:mainfrom
pavankumar-vh:fix/string-truncation-bug

Conversation

@pavankumar-vh

Copy link
Copy Markdown

Overview

Fix a critical bug in truncateStringWithMessage that could cause unexpected behavior when maxLength is smaller than the message/prefix/suffix length.

Bug Description

When maxLength is smaller than the message/prefix/suffix length, the old code computed a negative slice argument. String.slice treats negative end/start as counting from the end of the string, so instead of truncating to (roughly) maxLength, the function could silently return a chunk of the original string plus the truncation banner — defeating the whole point of bounding length.

Fix

Added Math.max(0, ...) guards to prevent negative slice lengths in all three truncation modes:

  • END mode: Math.max(0, maxLength - suffix.length)
  • START mode: Math.max(0, maxLength - prefix.length)
  • MIDDLE mode: Math.max(0, Math.floor((maxLength - middle.length) / 2))

Tests Added

Added 9 comprehensive test cases covering:

  • Normal truncation behavior for all three modes (END, START, MIDDLE)
  • Edge cases with negative/zero available length
  • Custom message handling
  • Empty string handling
  • Default behavior verification

All tests pass (28 total, 0 fail).

Files Changed

  • common/src/util/string.ts - Added safety guards to prevent negative slice indices
  • common/src/util/__tests__/string.test.ts - Added comprehensive test coverage

Scope

This change only touches common/ which is an approved contribution area per the Contributing Guide.

Bug Fixes:
1. Fix context window lookup in base-chat.ts: Handle missing/undefined model correctly
   - Change:  →
   - Prevents unnecessary lookup and improves clarity

2. Fix critical bug in truncateStringWithMessage: Prevent negative slice indices
   - Added Math.max(0, ...) guards to prevent negative slice lengths
   - Fixes potential runtime errors when maxLength < message length
   - Applies to all truncation modes (START, END, MIDDLE)

3. Add comprehensive tests for truncateStringWithMessage
   - Added 9 test cases covering edge cases
   - Tests for negative/zero available length scenarios
   - Tests for all truncation modes (START, END, MIDDLE)
   - Tests for custom messages and empty strings

All changes are in approved contribution areas (agents/, common/) and improve code safety.
… indices

When maxLength is smaller than the message/prefix/suffix length, the old code
computed a negative slice argument. String.slice treats negative end/start as
counting from the end of the string, so instead of truncating to (roughly)
maxLength, the function could silently return a chunk of the original string
plus the truncation banner — defeating the whole point of bounding length.

Added Math.max(0, ...) guards to prevent negative slice lengths in all three
truncation modes (END, START, MIDDLE).

Also added comprehensive tests covering:
- Normal truncation behavior for all three modes
- Edge cases with negative/zero available length
- Custom message handling
- Empty string handling
@codebuff-team

Copy link
Copy Markdown
Contributor

Good catch on the underlying issue — str.slice(0, maxLength - suffix.length) with a negative argument does count from the end rather than truncating, so for small maxLength values the function silently returns most of the original string plus the truncation banner. The Math.max(0, ...) guards for END and START modes in common/src/util/string.ts fix that correctly, and common/ is in scope per the contributing guide.

However, the MIDDLE-mode fix doesn't actually solve the problem it claims to. When maxLength is small enough that length clamps to 0, str.slice(-length) becomes str.slice(-0), and -0 === 0 in JS, so slice(-0) is identical to slice(0) — it returns the entire string, not an empty one. So for small maxLength in MIDDLE mode you still get the whole original string plus the truncation banner stuffed in the middle, which is exactly the bug this PR is meant to fix.

The added test 'should handle negative available length for MIDDLE mode' doesn't catch this because it only asserts result.toContain('TRUNCATED DUE TO LENGTH'), which is true regardless of whether the string was actually truncated. Please add an assertion checking the total output length (or that the head/tail portions are actually empty) to catch this, and fix the MIDDLE branch, e.g. by using length > 0 ? str.slice(-length) : ''.

Right instinct, solid fix for two of three branches, but the MIDDLE-mode case — arguably the trickiest of the three — is still broken, and the test suite as written wouldn't have caught it.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants